Repository navigation
Conversation
|
There was a problem hiding this comment.
Hi @chiliec, welcome! This is your first contribution here, thank you for taking the time to dig into #655, write it up so clearly and cover both trigger paths with a test that fails on develop.
Honoring the declaration surfaces two regressions for existing internal=True declarations: a declared delay stops being waited, and on the async engine the first trigger on a fresh machine is dropped. Details inline.
| return self | ||
| return BoundEvent(id=self.id, name=self.name, delay=self.delay, _sm=instance) | ||
| return BoundEvent( | ||
| id=self.id, name=self.name, delay=self.delay, internal=self.internal, _sm=instance |
There was a problem hiding this comment.
issue (blocking): Carrying internal through makes a declared delay silently inert: the internal queue never checks execution_time (only Phase 3 does), so Event(dark.to(lit), delay=50, internal=True) now fires at 0 ms, while on develop it waited 50 ms. That is exactly the declaration in test_accept_delay_and_internal and the example right above the new release note. Either hold internal triggers until their due time in both engines, or reject delay together with internal=True and say so in the note. raise_(..., delay=) has the same hole today, so a follow-up could cover it; #665 is rewriting the due-time checks, worth aligning with it.
| delay = ( | ||
| delay if delay else know_event and know_event.delay or 0 | ||
| ) # first the param, then the event, or 0 | ||
| internal = internal or bool(know_event and know_event.internal) |
There was a problem hiding this comment.
issue (blocking): On the async engine the first await sm.go() on a fresh machine now drops the event: Phase 1 pops it from the internal queue while the configuration is still empty (the __initial__ external event has not run yet), so it matches nothing, go() returns None and the machine stays in a. On develop the same declaration worked because the event went external, behind __initial__. docs/async.md promises the first send() activates the initial state automatically. Activate the initial configuration before an internal put from outside the loop, or process __initial__ before draining the internal queue when the configuration is empty.
|
|
||
| `Event(a.to(b), internal=True)` kept `internal` on the declaration, but neither `sm.go()` nor | ||
| `sm.send("go")` carried it to the queue, so the event always went to the external queue. Both | ||
| paths now honor the declaration and put the event on the internal queue. |
There was a problem hiding this comment.
suggestion (non-blocking): Both engines collect the processing result only from external events, so sm.go() on a declared-internal event now returns None instead of the callback result (verified: on_go returning "go-result" gives None once internal=True is declared). A class-level flag changing the return contract of every caller is worth one sentence here.
| assert BeaconsOfGondor().light.delay == 50 | ||
|
|
||
| @pytest.mark.parametrize("trigger", ["method", "send"]) | ||
| def test_declared_internal_puts_on_the_internal_queue(self, trigger): |
There was a problem hiding this comment.
suggestion (non-blocking): Run this through sm_runner so the async engine is covered too (CLAUDE.md, "Testing both sync and async engines"); the async path is where the declaration misbehaves (see the comment on send). A case that triggers before activate_initial_state() would have caught the dropped event.
| @@ -71,6 +71,31 @@ class BeaconsOfGondor(StateChart): | |||
| assert BeaconsOfGondor.light.delay == 50 | |||
| assert BeaconsOfGondor().light.delay == 50 | |||
There was a problem hiding this comment.
nitpick (non-blocking): The bound side of internal has no direct assertion.
| assert BeaconsOfGondor().light.delay == 50 | |
| assert BeaconsOfGondor().light.delay == 50 | |
| assert BeaconsOfGondor().light.internal is True |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #663 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 52 52
Lines 5582 5583 +1
Branches 883 883
=========================================
+ Hits 5582 5583 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks again @chiliec. To get this into 3.2.2 without making you chase the engine issues from the review, I carried your commit, with your authorship, into #668 and built the rest on top of it: an internal event triggered from outside a callback now goes to the external queue (so it returns its result and works on a fresh async machine), |
2 participants
Fixes #655.
Event(a.to(b), internal=True)was preserved on the declaration but never reached the queue:Event.__get__built theBoundEventwithoutinternal, sosm.go()always put on the external queue.StateChart.sendreaddelayfrom the declared event but notinternal, sosm.send("go")ignored it too.Fix: pass
internalthrough in__get__, and insendfall back to the declared event'sinternalwhen the caller did not passinternal=True(same precedence asdelay).Per the issue notes: dropped the "
internalstill has no effect at trigger time" caveat fromdocs/releases/3.2.2.md, added a 3.2.2 release-notes entry, and listed the declarative form in the queue table indocs/processing_model.md.Tests
TestExplicitEvent.test_declared_internal_puts_on_the_internal_queue(parametrized oversm.light()andsm.send("light")) records theinternalflag passed to the engine'sput. It fails ondevelopfor both paths ([False]) and passes with this change.uv run pytest: 2020 passed, 144 skipped, 44 xfaileduv run ruff check/ruff format --check/mypy statemachine: clean